Skip to content

gh-155811: Add a seqcount to gc_stats to prevent torn reads - #155828

Open
maurycy wants to merge 2 commits into
python:mainfrom
maurycy:gc-mon-seq
Open

gh-155811: Add a seqcount to gc_stats to prevent torn reads#155828
maurycy wants to merge 2 commits into
python:mainfrom
maurycy:gc-mon-seq

Conversation

@maurycy

@maurycy maurycy commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

See #155811 for the context.

tl;dr get_gc_stats reads the stats without pausing the target (by design), so it can return torn data.

The PR adds a seqcount to gc_stats atomically increased by the writer around the stats update on every collection, where odd means it's in progress.

gcmon is the main consumer.

There's no retry, as per #155811 (comment). It's not needed. gc.collect() does not happen that often:

2026-08-15T10:53:48.504627000+0200 maurycy@gimel /Users/maurycy/work/cpython (gc-mon-seq 1291568?) % sudo ./python.exe gc_seq_collision.py 
successes=1966598 failures=191
GC stats changed while being read; retry later

For:

import subprocess
import sys
import time

import _remote_debugging

target = """
import gc
import os
import time

print(os.getpid(), flush=True)
while True:
    gc.collect(0)
    time.sleep(0.01)
"""

p = subprocess.Popen(
    [sys.executable, "-c", target],
    stdout=subprocess.PIPE,
    text=True,
)
try:
    pid = int(p.stdout.readline())
    monitor = _remote_debugging.GCMonitor(pid, debug=True)
    successes = 0
    failures = 0
    messages = set()
    deadline = time.monotonic() + 10.0
    while time.monotonic() < deadline:
        try:
            monitor.get_gc_stats(all_interpreters=False)
            successes += 1
        except RuntimeError as exc:
            failures += 1
            messages.add(str(exc))
    print(f"successes={successes} failures={failures}")
    for message in sorted(messages):
        print(message)
finally:
    p.terminate()
    p.wait()

I'm not sure what's optimal non-hacky test. We haven't had one in #152448. To be honest, it's implicitly tested by the current happy-path tests in test_gc_stats.

@maurycy

maurycy commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

cc @sergey-miryanov @nascheme

Comment thread Modules/_remote_debugging/gc_stats.c
@sergey-miryanov

Copy link
Copy Markdown
Contributor

I'm concerned that we now need to read memory three times with this approach:

struct gc_stats {
    uint32_t before_update_seq;
    struct gc_young_stats_buffer young;
    struct gc_old_stats_buffer old[2];
    uint32_t after_update_seq;
};

If before_update_seq and after_update_seq don't match, we have a torn read.

What do you think?

@maurycy

maurycy commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@sergey-miryanov I'd push back. :-)

successes=1966598 failures=191

is around ~200KHz.

I'm not sure if there's a guarantee about copy-order and monotonicity of process_vm_readv etc.

Also, assuming these guarantees, before_update_seq and after_update_seq might be racy? What if:

  1. Reader read before_update_seq (0)
  2. Writer incremented before_update_seq (1)
  3. Reader read after_update_seq (0)
  4. Writer incremented after_update_seq (2)

@sergey-miryanov

Copy link
Copy Markdown
Contributor

Yes, my variant will not work, I thought more about it - so drop it :)

@sergey-miryanov

Copy link
Copy Markdown
Contributor

What if:
Writer:

  1. Move update_seq to the begin of struct
  2. Write to update_seq

Reader:

  1. Read gc_stats (update_seq + stats)
  2. Read update_seq
  3. Compare update_seq(1) and update_seq(2)

@sergey-miryanov

Copy link
Copy Markdown
Contributor

Even better, we could pack both counters into a single 32-bit integer — using the lower 16 bits for before and the upper 16 bits for after. This would let us read both values with a single atomic load.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants